crates/cli/src/cli/simnet/mod.rs (2)

15-18: Consider more graceful fallback instead of forcing process exit.
Currently, if nestable_block_on fails, the thread logs the error, sleeps, and then calls std::process::exit(1). This approach abruptly terminates the entire process, which might hinder debugging or partial recovery scenarios.

Consider returning the error to the main thread or providing a retry mechanism before exiting:

 if let Err(e) = hiro_system_kit::nestable_block_on(future) {
     error!(ctx_cloned.expect_logger(), "{e}");
     std::thread::sleep(std::time::Duration::from_millis(500));
-    std::process::exit(1);
+    // Possibly propagate this error to the main thread or support a retry count:
+    return Err(e);
 }

---

24-29: Separate the TUI from logging logic more cleanly.
The current code toggles between log_events and tui::simnet::start_app based on cmd.no_tui. Although this works, merging the logic might help unify how events are handled (e.g., common filtering, shared event queue).

crates/core/src/simnet/mod.rs (1)

71-72: Provide additional logs or fallback for server startup failure.
This block forcibly exits the process if the simulation server cannot start. While it may be appropriate, consider logging the cause or providing a fallback mechanism.

crates/cli/src/tui/simnet.rs (2)

158-225: Limit or rotate stored log events to prevent unbounded memory usage.
Currently, the TUI stores incoming events in a VecDeque without an explicit maximum size, which can lead to high memory usage over time if the simulation runs for an extended period.

To mitigate this:

// For example, keep only the latest 10,000 events:
const MAX_EVENTS: usize = 10_000;
if app.events.len() >= MAX_EVENTS {
    app.events.pop_back();
}

---

211-223: Validate keyboard input.
All key events are assumed to be valid within the TUI, but spurious or unknown keys might appear. Consider a default case for logging unknown keys or ignoring them gracefully.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

crates/cli/src/tui/simnet.rs (3)

24-35: Consider supporting multiple theme variations or external configuration.
Currently, the ColorTheme struct is hard-coded to a specific palette (tailwind::EMERALD). If theming becomes more advanced in the future or if users request color customization, you might consider allowing a parameter or a configuration-driven approach to handle multiple themes.

---

234-234: Avoid calling app.set_colors() on every render.
app.set_colors() is invoked on every UI render loop, but it only reassigns a static color palette. Consider moving it to the initialization phase, or call it only when the user actually changes the theme.

- app.set_colors();
+ // Only call app.set_colors() if theming is toggled or changed at runtime

---

304-320: Potential performance overhead in render_slots.
For large terminal dimensions, generating and concatenating a big string of ▮ / ▯ characters might be expensive. Though acceptable for smaller screens, consider short-circuiting or limiting the output if performance becomes an issue for extremely large terminals.

crates/cli/src/cli/simnet/mod.rs (2)

16-18: Avoid hard process exit.
std::process::exit(1) bypasses typical cleanup, dropping, and unwinding. If feasible, consider returning an error instead and letting upstream logic handle it gracefully.

---

37-75: Consider a mechanism to stop logging gracefully.
log_events loops until the channel is closed, which can be fine, but in situations where the simulator needs to stop early or the user cancels, some interruption mechanism may be desirable.

crates/cli/src/cli/mod.rs (2)

73-77: Ensure consistent naming of flags and clarity in help text.
The flags no-tui and debug are straightforward, but consider clarifying them in the help text:

• --no-tui: "Use plain logs instead of the TUI dashboard."
• --debug: "Enable debug-level logs."
  This can improve usability for new users.

---

117-125: Enhance user guidance for generated completion scripts.
In addition to printing the file name, you could provide an explicit shell command snippet or link to the official docs for each shell so users can quickly enable completions.

crates/core/src/simnet/mod.rs (1)

86-117: Refactor transaction processing into a separate function.

The transaction processing logic is complex and could benefit from being moved into a dedicated function for better maintainability.

+fn process_transaction(
+    tx: VersionedTransaction,
+    ctx: &mut GlobalState,
+    rpc_client: &RpcClient,
+    simnet_events_tx: &Sender<SimnetEvent>,
+) {
+    tx.verify_with_results();
+    let tx = tx.into_legacy_transaction().unwrap();
+    let message = &tx.message;
+    
+    for instruction in &message.instructions {
+        if instruction.program_id_index as usize >= message.account_keys.len() {
+            unreachable!();
+        }
+        let program_id = &message.account_keys[instruction.program_id_index as usize];
+        if ctx.svm.get_account(&program_id).is_none() {
+            let res = rpc_client.get_account(&program_id);
+            let event = match res {
+                Ok(account) => {
+                    let _ = ctx.svm.set_account(*program_id, account);
+                    SimnetEvent::AccountUpdate(Local::now(), program_id.clone())
+                }
+                Err(e) => SimnetEvent::ErroLog(
+                    Local::now(),
+                    format!("unable to retrieve account: {}", e),
+                ),
+            };
+            if let Err(e) = simnet_events_tx.send(event) {
+                eprintln!("Failed to send account event: {}", e);
+            }
+        }
+    }
+    let res = ctx.svm.send_transaction(tx);
+}

Then use it in the main loop:

 while let Ok(tx) = mempool_rx.try_recv() {
-    // ... existing transaction processing code ...
+    process_transaction(tx, &mut ctx, &rpc_client, &simnet_events_tx);
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

crates/cli/src/cli/simnet/mod.rs (1)

8-30: Improve error handling with more context.

The error handling could be enhanced by providing more context about the failure points:

Apply this diff to improve error handling:

-        .map_err(|e| format!("{}", e))?;
+        .map_err(|e| format!("Failed to spawn simnet thread: {}", e))?;

-    handle.join().map_err(|_e| format!("unable to terminate"))?
+    handle.join().map_err(|e| format!("Failed to join simnet thread: {:?}", e))?

crates/cli/src/cli/mod.rs (1)

110-115: Remove unnecessary async marker.

The function is marked as async but doesn't use any async operations. The implementation is synchronous.

Apply this diff to remove the async marker:

-async fn handle_command(opts: Opts, ctx: &Context) -> Result<(), String> {
+fn handle_command(opts: Opts, ctx: &Context) -> Result<(), String> {

crates/core/src/simnet/mod.rs (1)

70-73: Handle server startup errors gracefully.

The server startup error handling could be improved by providing more context about the failure.

Apply this diff to improve error handling:

-        let Ok(server) = res else {
-            std::process::exit(1);
-        };
+        let server = res.map_err(|e| {
+            eprintln!("Failed to start server: {}", e);
+            std::process::exit(1);
+        })?;

crates/cli/src/tui/simnet.rs (1)

322-366: Limit event queue size and make column widths configurable.

Consider the following improvements:

1. Add a maximum size for the event queue to prevent unbounded memory growth.
2. Make column widths configurable or calculate them based on terminal width.

Apply this diff to limit queue size:

 impl App {
     fn new(simnet_events_rx: Receiver<SimnetEvent>, include_debug_logs: bool) -> App {
+        const MAX_EVENTS: usize = 1000;
         App {
             state: TableState::default().with_selected(0),
             scroll_state: ScrollbarState::new(5 * ITEM_HEIGHT),
             colors: ColorTheme::new(&palette::tailwind::EMERALD),
             simnet_events_rx,
             clock: Clock::default(),
             epoch_info: EpochInfo {
                 epoch: 0,
                 slot_index: 0,
                 slots_in_epoch: 0,
                 absolute_slot: 0,
                 block_height: 0,
                 transaction_count: None,
             },
-            events: VecDeque::new(),
+            events: VecDeque::with_capacity(MAX_EVENTS),
             include_debug_logs,
         }
     }
+
+    fn add_event(&mut self, event_type: EventType, dt: DateTime<Local>, log: String) {
+        self.events.push_front((event_type, dt, log));
+        if self.events.len() > MAX_EVENTS {
+            self.events.pop_back();
+        }
+    }
 }

crates/cli/src/cli/simnet/mod.rs (1)

32-76: 🛠️ Refactor suggestion

Handle logger access safely.

The function uses expect_logger() which could panic at runtime. Consider handling the case where logger is not available.

Apply this diff to handle logger access safely:

-    info!(
-        ctx.expect_logger(),
-        "Surfpool: The best place to train before surfing Solana"
-    );
+    if let Some(logger) = ctx.logger.as_ref() {
+        info!(
+            logger,
+            "Surfpool: The best place to train before surfing Solana"
+        );
+    }

Apply similar changes to other logger usages in the function.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

fn log_events(simnet_events_rx: Receiver<SimnetEvent>, include_debug_logs: bool, ctx: &Context) {
    if let Some(logger) = ctx.logger.as_ref() {
        info!(
            logger,
            "Surfpool: The best place to train before surfing Solana"
        );
    }
    while let Ok(event) = simnet_events_rx.recv() {
        match event {
            SimnetEvent::AccountUpdate(_, account) => {
                info!(
                    ctx.expect_logger(),
                    "Account retrieved from Mainnet {}", account
                );
            }
            SimnetEvent::EpochInfoUpdate(epoch_info) => {
                info!(
                    ctx.expect_logger(),
                    "Connection established. Epoch {}, Slot {}.",
                    epoch_info.epoch,
                    epoch_info.slot_index
                );
            }
            SimnetEvent::ClockUpdate(clock) => {
                info!(ctx.expect_logger(), "Slot #{} ", clock.slot);
            }
            SimnetEvent::ErroLog(_, log) => {
                error!(ctx.expect_logger(), "{} ", log);
            }
            SimnetEvent::InfoLog(_, log) => {
                info!(ctx.expect_logger(), "{} ", log);
            }
            SimnetEvent::WarnLog(_, log) => {
                warn!(ctx.expect_logger(), "{} ", log);
            }
            SimnetEvent::DebugLog(_, log) => {
                if include_debug_logs {
                    debug!(ctx.expect_logger(), "{} ", log);
                }
            }
            SimnetEvent::TransactionReceived(_, _transaction) => {
                info!(ctx.expect_logger(), "Transaction received");
            }
            SimnetEvent::BlockHashExpired => {}
        }
    }
}

crates/cli/src/cli/mod.rs (1)

117-126: ⚠️ Potential issue

Add missing macro import.

The code uses the green! macro but it's not imported.

Add the following import at the top of the file:

+use crate::utils::green;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

use crate::utils::green;

fn generate_completion_helpers(cmd: &Completions) -> Result<(), String> {
    let mut app = Opts::command();
    let file_name = cmd.shell.file_name("surfpool");
    let mut file = File::create(file_name.clone())
        .map_err(|e| format!("unable to create file {}: {}", file_name, e))?;
    clap_complete::generate(cmd.shell, &mut app, "surfpool", &mut file);
    println!("{} {}", green!("Created file"), file_name.clone());
    println!("Check your shell’s docs for how to enable completions for surfpool.");
    Ok(())
}

crates/cli/src/tui/simnet.rs (1)

94-96: 🛠️ Refactor suggestion

Handle slot conversion safely.

Using unwrap() on the conversion could cause panics if the slot value doesn't fit in usize.

Apply this diff to handle the conversion safely:

-    pub fn slot(&self) -> usize {
-        self.clock.slot.try_into().unwrap()
+    pub fn slot(&self) -> usize {
+        self.clock.slot.try_into().unwrap_or(0)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    pub fn slot(&self) -> usize {
        self.clock.slot.try_into().unwrap_or(0)
    }